Fix status item menu bar observation for token account snapshots (#2283) - #2511
Fix status item menu bar observation for token account snapshots (#2283)#2511Yuxin-Qiao wants to merge 4 commits into
Conversation
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed July 31, 2026, 1:22 AM ET / 05:22 UTC. ClawSweeper reviewWhat this changesThe PR makes token-account snapshot collections and their mutable usage, error, and source-label state participate in CodexBar’s menu-bar icon redraw observation, and adds focused regression tests. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 3 items remain This remains a useful, focused fix for the stale menu-bar indicator path. The prior signature and observation findings appear addressed in the current PR head, but the PR still needs after-fix evidence from a freshly built CodexBar bundle showing an account-snapshot update actually redraws the menu-bar icon; reported tests and checks alone do not meet that merge gate. Priority: P2 Review scores
Verification
How this fits togetherProvider refreshes populate flowchart LR
A[Provider usage refresh] --> B[Token account snapshots]
B --> C[Usage store observation]
C --> D[Icon work signature]
D --> E[Status bar icon redraw]
E --> F[Visible usage indicator]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the focused observation and signature fix, then attach redacted runtime proof from a freshly built CodexBar bundle showing the menu-bar indicator changes after an account snapshot changes. Do we have a high-confidence way to reproduce the issue? No high-confidence live reproduction was established in this read-only review. The source path and regression tests make the stale-signature mechanism reproducible from code, but not yet demonstrated in a freshly built running app. Is this the best way to solve the issue? Yes, with proof pending: observing the snapshot collections and including the mutable values that drive icon output is the narrowest maintainable fix for a redraw gate that previously ignored those changes. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8ef86077e70a. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce475ecc1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _ = self.accountSnapshots | ||
| _ = self.codexAccountSnapshots |
There was a problem hiding this comment.
Once the initial icon signature is stored, a refresh that changes only either per-account collection triggers this observation but still does no icon work: observeStoreIconChanges() recomputes storeIconObservationSignature() and returns because that signature reads store.snapshot(for:) and never accountSnapshots or codexAccountSnapshots. The status indicator therefore remains stale in the scenario these dependencies are intended to fix; include the relevant account state in the signature/render source (or publish it into snapshots) and test through the controller's signature gate rather than only asserting raw Observation invalidation.
AGENTS.md reference: AGENTS.md:L4-L5
Useful? React with 👍 / 👎.
…status item updates (steipete#2283)
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b7e280360
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let accountPart = accountSnapshots.map { snap in | ||
| let label = snap.account.label | ||
| return "\(snap.account.id.uuidString):\(label)" |
There was a problem hiding this comment.
The new signature projection is fresh evidence that the previously reported gate remains incomplete: an ordinary refresh replaces an existing TokenAccountUsageSnapshot with updated snapshot, error, or sourceLabel values while preserving its ID and label, so Observation fires but this string remains unchanged and observeStoreIconChanges() returns without updating the status item. The Codex projection has the same problem because it retains only ID, email, and active state; include the mutable fields that are intended to trigger icon work and cover a same-account refresh rather than only insertion into an empty collection.
Useful? React with 👍 / 👎.
|
@clawsweeper re-review Here is the technical summary and evidence for PR #2511: Problem & Fix Summary
Test Coverage & Evidence
|
clawsweeper
Bot
added
the
merge-risk: 🚨 other
…teipete#2511) The accountSnapshotSignature only tracked stable identity fields, so replacing an account snapshot's usage, error, or source label did not invalidate the observation token. Extend the signature to include the full snapshot state (primary/secondary/tertiary RateWindow details, error, sourceLabel) so icon redraws are triggered when mutable data changes. Add regression tests for: - Account snapshot usage replacement - Account snapshot error replacement - Codex account snapshot replacement Co-authored-by: OpenAI Codex <codex@openai.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
@clawsweeper The mutable snapshot state has been added to the work signature. Specifically:
Please re-review. |
clawsweeper
Bot
added
rating: 🦪 silver shellfish
1 participant
Summary
When account snapshots (
accountSnapshotsorcodexAccountSnapshots) were updated, the status item menu bar observation token (iconObservationToken) inUsageStorewas missing dependencies on these collections. This caused the status item menu bar icon to remain stale until another tracked property was invalidated.Additionally, the work signature (
accountSnapshotSignature) only tracked stable identity fields (id, label, email, isActive). When an existing snapshot's mutable state (usage percentages, errors, source labels) changed without the identity fields changing, the signature stayed the same and the status item icon was not refreshed.Changes Made
_ = self.accountSnapshotsand_ = self.codexAccountSnapshotstoiconObservationTokenfor observation invalidation.accountSnapshotSignatureto include full mutable state per snapshot (primary/secondary/tertiaryRateWindowdetails, error, sourceLabel) so icon redraws are triggered when usage data, errors, or source labels change.accountSnapshotsarray mutation triggers observation invalidation.codexAccountSnapshotsarray mutation triggers observation invalidation.Verification
swift test --filter StatusItemIconObservationSignatureTests(25/25 passed).make check(0 format/lint violations).Fixes #2283